src/services/webhooks/deliveryWorker.js (1)

109-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Nice touch on the auto-disable safety valve — let's just not copy-paste it. 🙂

The exact same auto-disable block (set isActive=false, disabledAt, disabledReason, log warn) is repeated verbatim in the non-2xx branch here and again in the catch branch at Lines 145-154. Duplicated logic like this tends to drift over time (someone fixes one path and forgets the other), so extracting a tiny helper keeps both failure paths honest.

♻️ Suggested helper extraction

+async function maybeAutoDisableEndpoint(endpoint) {
+  if (endpoint.consecutiveFailures < MAX_CONSECUTIVE_FAILURES_TO_DISABLE) return;
+  endpoint.isActive = false;
+  endpoint.disabledAt = new Date();
+  endpoint.disabledReason = `Auto-disabled after ${endpoint.consecutiveFailures} consecutive delivery failures`;
+  await endpoint.save();
+  logger.warn(
+    { endpointId: endpoint._id, consecutiveFailures: endpoint.consecutiveFailures },
+    "Webhook endpoint auto-disabled due to sustained failures"
+  );
+}

Then call await maybeAutoDisableEndpoint(endpoint); in both branches.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/services/webhooks/deliveryWorker.js` around lines 109 - 119, Extract the
repeated auto-disable logic from the non-2xx and catch failure paths into a
shared async helper, such as maybeAutoDisableEndpoint, preserving the existing
threshold check, endpoint updates, save operation, and warning log. Replace both
duplicated blocks with await maybeAutoDisableEndpoint(endpoint) so both branches
use the same behavior.

app.js (1)

184-193: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Router mount + test-guarded worker start look correct. The NODE_ENV !== "test" guard matches the .env.example contract (worker must not run background jobs under test).

One optional follow-up: webhookWorker holds a stop() handle but I don't see it invoked on shutdown (SIGTERM/SIGINT). Wiring it into graceful shutdown avoids leaked timers/half-sent deliveries during redeploys, if you have a shutdown hook elsewhere.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app.js` around lines 184 - 193, Wire the `webhookWorker` stop handle into the
existing graceful shutdown path for SIGTERM/SIGINT, invoking `stop()` before
process termination. Preserve the current `NODE_ENV !== "test"` startup guard
and ensure shutdown remains safe when `webhookWorker` is null.

src/controllers/webhookController.js (2)

11-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused helper: requireAdmin is never called.

Every handler enforces authorization inline via req.user.role !== "admin" && endpoint.owner.toString() !== ..., so this function is dead code. Either wire it up where admin-only behavior is intended, or remove it to avoid confusing future maintainers about which authorization pattern is actually enforced.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/controllers/webhookController.js` around lines 11 - 20, Remove the unused
requireAdmin helper from the controller, since authorization is currently
enforced inline by the handlers. Do not alter the existing inline authorization
checks or introduce a second authorization pattern.

---

270-281: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Unbounded/unvalidated pagination inputs.

page/limit come straight from req.query with no numeric validation or upper bound. A non-numeric value produces NaN for skip/limit (likely a driver error caught as a generic 500), and there's nothing preventing limit=100000+ from doing a very large scan/transfer.

♻️ Suggested guard

-    const { status, page = 1, limit = 20 } = req.query;
+    const { status } = req.query;
+    const page = Math.max(1, parseInt(req.query.page, 10) || 1);
+    const limit = Math.min(100, Math.max(1, parseInt(req.query.limit, 10) || 20));
     const query = { endpoint: endpoint._id };
     if (status) query.status = status;
 
-    const skip = (parseInt(page) - 1) * parseInt(limit);
+    const skip = (page - 1) * limit;
     const [deliveries, total] = await Promise.all([
       WebhookDelivery.find(query)
         .sort({ createdAt: -1 })
         .skip(skip)
-        .limit(parseInt(limit)),
+        .limit(limit),
       WebhookDelivery.countDocuments(query),
     ]);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/controllers/webhookController.js` around lines 270 - 281, Validate and
normalize the page and limit inputs in the delivery query flow before
calculating skip, rejecting non-numeric or non-positive values and enforcing a
reasonable maximum limit. Update the pagination logic around
WebhookDelivery.find and countDocuments so invalid requests receive the
controller’s established client-error response, while valid requests continue
using the normalized values for skip and limit.

src/services/webhooks/webhookService.js (1)

120-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Dead/buggy helper — not used by the delivery worker and contains a placeholder bug.

buildDeliveryHeaders sets "X-DeenBridge-Event": undefined (to be "set per-delivery") and never sets X-DeenBridge-Event-Id. Per the supplied deliveryWorker.js context snippet, the actual delivery worker builds its headers manually inline rather than calling this function, so this exported helper appears to be dead/duplicated logic that has drifted out of sync with the real header set (missing the event-id header) and would need extra wiring to actually work if someone picked it up later.

Recommend either removing this function or fixing it to match the real header contract and having deliveryWorker.js call it, so there's a single source of truth for delivery headers.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/services/webhooks/webhookService.js` around lines 120 - 133, The exported
buildDeliveryHeaders helper is unused and contains an undefined event header
while omitting the event-id header. Remove this dead helper, or make it the
single source of truth by matching the delivery worker’s complete header
contract and updating deliveryWorker.js to call it for every delivery.

src/utils/ssrfGuard.js (1)

27-37: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Minor: IPv6 private-range detection is too coarse, and isIPv6Loopback is unused.

ip.startsWith("fc") || ip.startsWith("fe") (Line 29) will match any IPv6 address in fe00::–feff::, not just the intended fe80::/10 link-local range, so some legitimate public-ish IPv6 literals could be rejected. This fails safe (over-blocks) so it's not exploitable, just imprecise. Also isIPv6Loopback (Lines 35-37) is defined but never called.

Only checking resolve4 also means an endpoint hostname that only has an AAAA record has no IPv6-specific private-range check at all (it just fails DNS resolution and gets rejected as invalid, which is safe but not correct for legitimate IPv6-only hosts).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/ssrfGuard.js` around lines 27 - 37, The IPv6 classification in
isPrivateOrLoopback is overly broad and leaves isIPv6Loopback unused. Replace
prefix checks with precise IPv6 range validation for fc00::/7 and fe80::/10,
reuse isIPv6Loopback where appropriate, and update hostname resolution to
inspect AAAA records alongside resolve4 so valid IPv6-only hosts receive the
same private-range checks.

src/models/WebhookDelivery.js (1)

61-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Add a compound index covering the listDeliveries sort pattern.

webhookController.js's listDeliveries filters by { endpoint, status? } and sorts by createdAt: -1. The existing { endpoint: 1, status: 1 } index doesn't cover the sort, so Mongo will need an in-memory sort once an endpoint accumulates many delivery records.

♻️ Suggested additional index

 webhookDeliverySchema.index({ endpoint: 1, status: 1 });
 webhookDeliverySchema.index({ status: 1, nextAttemptAt: 1 });
+webhookDeliverySchema.index({ endpoint: 1, createdAt: -1 });

As per path instructions, "Flag schema changes that break existing documents, missing indexes for new query patterns, and TTL indexes that could delete durable records."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/models/WebhookDelivery.js` around lines 61 - 62, Update
webhookDeliverySchema indexes to add a compound index matching the
listDeliveries query pattern: endpoint and optional status equality filters
followed by createdAt descending sort. Preserve the existing indexes and ensure
the new index supports efficient sorted retrieval without an in-memory sort.

Source: Path instructions

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes